Add getScanner() method to transaction interfaces #2698

brfrn169 · 2025-05-26T14:03:11Z

Description

This PR adds the getScanner() method to the transaction interfaces. It does not include the actual implementations; those will be provided in subsequent PRs.

Note that we are working on this feature in the add-scanner-api-to-transaction-abstraction feature branch, as this change may cause compile errors in dependent projects. Once all implementations are completed, we will merge the feature branch into the master branch.

Related issues and/or PRs

N/A

Changes made

Added the getScanner() method and the Scanner interface to CrudOperable.
Added the getScanner() method and the Scanner interface to TransactionManagerCrudOperable.
Added the getScanner() method and the Scanner interface to TransactionCrudOperable.
Added integration tests for the getScanner() method.

Checklist

The following is a best-effort checklist. If any items in this checklist are not applicable to this PR or are dependent on other, unmerged PRs, please still mark the checkboxes after you have read and understood each item.

I have commented my code, particularly in hard-to-understand areas.
I have updated the documentation to reflect the changes.
I have considered whether similar issues could occur in other products, components, or modules if this PR is for bug fixes.
Any remaining open issues linked to this PR are documented and up-to-date (Jira, GitHub, etc.).
Tests (unit, integration, etc.) have been added for the changes.
My changes generate no new warnings.
Any dependent changes in other PRs have been merged and published.

Additional notes (optional)

N/A

Release notes

N/A

Copilot

Pull Request Overview

This PR introduces a new getScanner(Scan) method to the transaction CRUD interfaces, provides a base scanner implementation, and wires up stub or delegated implementations throughout the core and service layers.

Extended CrudOperable, TransactionCrudOperable, and TransactionManagerCrudOperable with getScanner(Scan) and a nested Scanner interface.
Added AbstractCrudOperableScanner (and two specialized subclasses) to provide a reusable iterator-based scanner implementation.
Implemented stub or delegated getScanner overrides across transaction, manager, and service classes; added placeholder integration tests.

Reviewed Changes

Copilot reviewed 29 out of 29 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
core/src/main/java/com/scalar/db/transaction/consensuscommit/TwoPhaseConsensusCommit.java	Added stub `getScanner` method
core/src/main/java/com/scalar/db/transaction/consensuscommit/ConsensusCommitManager.java	Added stub `getScanner` method
core/src/main/java/com/scalar/db/transaction/consensuscommit/ConsensusCommit.java	Added stub `getScanner` method
core/src/main/java/com/scalar/db/service/TwoPhaseCommitTransactionService.java	Delegated `getScanner` to manager; updated deprecations
core/src/main/java/com/scalar/db/service/TransactionService.java	Delegated `getScanner` to manager; updated deprecations
core/src/main/java/com/scalar/db/common/StateManagedTwoPhaseCommitTransactionManager.java	Added active-check + delegate `getScanner`
core/src/main/java/com/scalar/db/common/StateManagedDistributedTransactionManager.java	Added active-check + delegate `getScanner`
core/src/main/java/com/scalar/db/common/DecoratedTwoPhaseCommitTransactionManager.java	Delegated `getScanner`
core/src/main/java/com/scalar/db/common/DecoratedTwoPhaseCommitTransaction.java	Delegated `getScanner`
core/src/main/java/com/scalar/db/common/DecoratedDistributedTransactionManager.java	Delegated `getScanner`
core/src/main/java/com/scalar/db/common/DecoratedDistributedTransaction.java	Delegated `getScanner`
core/src/main/java/com/scalar/db/common/ActiveTransactionManagedTwoPhaseCommitTransactionManager.java	Synchronized override `getScanner`
core/src/main/java/com/scalar/db/common/ActiveTransactionManagedDistributedTransactionManager.java	Synchronized override `getScanner`
core/src/main/java/com/scalar/db/common/AbstractTransactionManagerCrudOperableScanner.java	Added abstract manager-scanner adapter
core/src/main/java/com/scalar/db/common/AbstractTransactionCrudOperableScanner.java	Added abstract transaction-scanner adapter
core/src/main/java/com/scalar/db/common/AbstractCrudOperableScanner.java	Implemented shared scanner iterator logic
core/src/main/java/com/scalar/db/api/TransactionManagerCrudOperable.java	Extended API with `getScanner` and nested `Scanner`
core/src/main/java/com/scalar/db/api/TransactionCrudOperable.java	Extended API with `getScanner` and nested `Scanner`
core/src/main/java/com/scalar/db/api/CrudOperable.java	Added `getScanner` to base CRUD API and nested `Scanner`
core/src/integration-test/java/com/scalar/db/transaction/jdbc/JdbcTransactionIntegrationTest.java	Added placeholder tests for `getScanner`

Comments suppressed due to low confidence (3)

core/src/integration-test/java/com/scalar/db/transaction/jdbc/JdbcTransactionIntegrationTest.java:46

These placeholder tests for getScanner have no assertions; consider implementing actual coverage or removing them until behavior is defined.

@Disabled("Implement later")

core/src/main/java/com/scalar/db/common/DecoratedTwoPhaseCommitTransactionManager.java:115

[nitpick] This override is boilerplate repeated across many classes; consider providing a default method in the interface to reduce duplication.

public Scanner getScanner(Scan scan) throws CrudException {

core/src/main/java/com/scalar/db/api/CrudOperable.java:61

[nitpick] Since most implementations throw UnsupportedOperationException, you could provide a default interface implementation that throws a consistent exception, avoiding the need to add stubs everywhere.

Scanner<E> getScanner(Scan scan) throws E;

Copilot · 2025-05-26T14:06:09Z

core/src/main/java/com/scalar/db/api/TransactionManagerCrudOperable.java

+   *     (e.g., a conflict error). You can retry the transaction from the beginning
+   * @throws CrudException if the transaction CRUD operation fails due to transient or nontransient
+   *     faults. You can try retrying the transaction from the beginning, but the transaction may
+   *     still fail if the cause is nontranient


The word "nontranient" is a typo; it should be spelled "nontransient".

Suggested change

* still fail if the cause is nontranient

* still fail if the cause is nontransient

I'll fix it in a separate PR.

Copilot · 2025-05-26T14:06:09Z

core/src/main/java/com/scalar/db/api/TransactionCrudOperable.java

+   *     (e.g., a conflict error). You can retry the transaction from the beginning
+   * @throws CrudException if the transaction CRUD operation fails due to transient or nontransient
+   *     faults. You can try retrying the transaction from the beginning, but the transaction may
+   *     still fail if the cause is nontranient


The word "nontranient" is a typo; it should be spelled "nontransient".

Suggested change

* still fail if the cause is nontranient

* still fail if the cause is nontransient

I'll fix it in a separate PR.

Copilot · 2025-05-26T14:06:10Z

core/src/main/java/com/scalar/db/transaction/consensuscommit/TwoPhaseConsensusCommit.java

      throw e;
    }
  }



[nitpick] The stub override lacks JavaDoc. Even a brief @throws UnsupportedOperationException note would clarify the intent.

Suggested change

/**

* This method is not yet implemented.

*

* @param scan the scan operation to perform

* @return nothing, as this method always throws an exception

* @throws UnsupportedOperationException always, as this method is a stub

*/

brfrn169 · 2025-05-26T14:11:17Z

core/src/main/java/com/scalar/db/api/CrudOperable.java

+   * @return a {@code Scanner} to iterate over the results
+   * @throws E if the transaction CRUD operation fails
+   */
+  Scanner<E> getScanner(Scan scan) throws E;


Added the getScanner() method to CrudOperable.

brfrn169 · 2025-05-26T14:11:40Z

core/src/main/java/com/scalar/db/api/CrudOperable.java

  void mutate(List<? extends Mutation> mutations) throws E;
+
+  /** A scanner abstraction for iterating results. */
+  interface Scanner<E extends TransactionException> extends AutoCloseable, Iterable<Result> {


Also, added the Scanner interface to CrudOperable.

brfrn169 · 2025-05-26T14:13:05Z

core/src/main/java/com/scalar/db/api/TransactionManagerCrudOperable.java

+   *     still fail if the cause is nontranient
+   */
+  @Override
+  Scanner getScanner(Scan scan) throws CrudConflictException, CrudException;


Also, added the getScanner() method and the Scanner interface to TransactionManagerCrudOperable.

brfrn169 · 2025-05-26T14:13:20Z

core/src/main/java/com/scalar/db/api/TransactionCrudOperable.java

+   *     still fail if the cause is nontranient
+   */
+  @Override
+  Scanner getScanner(Scan scan) throws CrudConflictException, CrudException;


Also, added the getScanner() method and the Scanner interface to TransactionCrudOperable.

komamitsu · 2025-05-27T02:15:01Z

core/src/main/java/com/scalar/db/common/AbstractCrudOperableScanner.java

+  @Override
+  @Nonnull
+  public Iterator<Result> iterator() {
+    if (scannerIterator == null) {


Based on this implementation, this class should be also @NotThreadSafe?

Do you think we should add @NotThreadSafe to abstract classes? Actually, we basically don’t do that at the moment.

komamitsu

LGTM! 👍

Torch3333 · 2025-05-28T05:43:21Z

core/src/main/java/com/scalar/db/api/CrudOperable.java

+    /**
+     * Returns the first result in the results.
+     *
+     * @return the first result in the results
+     * @throws E if the operation fails
+     */


The Javadoc was copied from com.scalar.db.api.Scanner, but I think it could be improved to be clear one() iterates over the results.

Suggested change

/**

* Returns the first result in the results.

*

* @return the first result in the results

* @throws E if the operation fails

*/

/**

* Returns the next result.

*

* @return an Optional containing the next result if available, or empty if no more results

* @throws E if the operation fails

*/

Fixed in 8ae4db1. Thanks!

Torch3333 · 2025-05-28T05:44:24Z

core/src/main/java/com/scalar/db/api/CrudOperable.java

+    /**
+     * Returns all the results.
+     *
+     * @return the list of {@code Result}s
+     * @throws E if the operation fails
+     */


Likewise to the comment above. I think the Javadoc should be more clear that it iterates over the results.

Suggested change

/**

* Returns all the results.

*

* @return the list of {@code Result}s

* @throws E if the operation fails

*/

/**

* Returns all remaining results.

*

* @return a List containing all remaining results

* @throws E if the operation fails

*/

Fixed in 8ae4db1. Thanks!

Torch3333

LGTM, thank you!

feeblefakie

LGTM! Thank you!

Add getScanner() method to transaction interfaces

e61e250

brfrn169 requested a review from Copilot May 26, 2025 14:03

brfrn169 self-assigned this May 26, 2025

brfrn169 requested a review from a team as a code owner May 26, 2025 14:03

brfrn169 requested review from Torch3333, feeblefakie and komamitsu and removed request for a team May 26, 2025 14:03

Copilot AI reviewed May 26, 2025

View reviewed changes

brfrn169 commented May 26, 2025

View reviewed changes

komamitsu reviewed May 27, 2025

View reviewed changes

brfrn169 requested a review from komamitsu May 27, 2025 04:54

komamitsu approved these changes May 27, 2025

View reviewed changes

Torch3333 reviewed May 28, 2025

View reviewed changes

Revise Javadoc

8ae4db1

brfrn169 requested a review from Torch3333 May 28, 2025 05:55

Torch3333 approved these changes May 28, 2025

View reviewed changes

feeblefakie approved these changes May 28, 2025

View reviewed changes

brfrn169 merged commit da1cb57 into add-scanner-api-to-transaction-abstraction May 28, 2025
55 checks passed

brfrn169 deleted the add-getScanner-to-transaction-interfaces branch May 28, 2025 07:23

	* still fail if the cause is nontranient
	* still fail if the cause is nontransient

+  /**
+   * This method is not yet implemented.
+   *
+   * @param scan the scan operation to perform
+   * @return nothing, as this method always throws an exception
+   * @throws UnsupportedOperationException always, as this method is a stub
+   */

Add getScanner() method to transaction interfaces #2698

Add getScanner() method to transaction interfaces #2698

Uh oh!

Conversation

brfrn169 commented May 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related issues and/or PRs

Changes made

Checklist

Additional notes (optional)

Release notes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI May 26, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI May 26, 2025

Choose a reason for hiding this comment

Uh oh!

brfrn169 May 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI May 26, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brfrn169 May 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

komamitsu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Torch3333 left a comment

Choose a reason for hiding this comment

Uh oh!

feeblefakie left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

brfrn169 commented May 26, 2025 •

edited

Loading

brfrn169 May 26, 2025 •

edited

Loading

brfrn169 May 26, 2025 •

edited

Loading